Cache lock changes #5608
Cache lock changes #5608
Conversation
✅ Deploy Preview for apollo-android-docs canceled.
|
| repeat(WORK_LOAD) { | ||
| apolloStore.writeOperation(query, data) | ||
| val data2 = apolloStore.readOperation(query) | ||
| Assert.assertEquals(data, data2) | ||
| } |
There was a problem hiding this comment.
Can you also add an integration test, that tests end to end using apolloClient.query() and the default dispatcher? (should be Dispatchers.Default IIRC). Are the results similar?
There was a problem hiding this comment.
Do you mean still in a microbenchmark or something else?
There was a problem hiding this comment.
I think so? Same thing but closer to the real scenario of executing a query. Probably using MockWebServer or so. I know it adds more variance but it's also what users are actually doing.
There was a problem hiding this comment.
Added in c8cee0f benches that execute queries in parallel. The results are a bit questionable:
| Memory | Sql | Memory then sql | |
|---|---|---|---|
| Before | 44,201,994 | 1,047,540,153 | 41,335,067 |
| After | 40,916,743 | 1,079,325,820 | 38,458,753 |
| Improvement | 7.4% | -3% | 6.9% |
| @Test | ||
| fun concurrentReadWritesMemory() { | ||
| concurrentReadWrites(MemoryCacheFactory()) | ||
| } |
There was a problem hiding this comment.
Are those uploaded to datadog automagically using the run-benchmarks script ? I would say so but I can't remember.
There was a problem hiding this comment.
I think so from what I read in the script 😅.
There was a problem hiding this comment.
Do we want that? I guess yes? Additional question is: do we want to track it in the dashboard? I don't think it's going to appear in the dashboard without manual datadog configuration.
There was a problem hiding this comment.
I'd say yes (and also yes for DataDog) Doesn't hurt to track it :) I'll have a look at the DD conf.
...ppleMain/kotlin/com/apollographql/apollo3/cache/normalized/api/internal/-cache-lock-apple.kt
Show resolved
Hide resolved
...commonMain/kotlin/com/apollographql/apollo3/cache/normalized/api/internal/OptimisticCache.kt
Show resolved
Hide resolved
| if (record != null) { | ||
| if (cacheHeaders.hasHeader(EVICT_AFTER_READ)) { | ||
| recordDatabase.delete(key) | ||
| // A lock is only needed if there is a nextCache |
There was a problem hiding this comment.
Are we 100% certain of that? SQLDelight queries being thread safe no matter what underlying driver is used? A comment to the relevant doc/reference would be handy.
There was a problem hiding this comment.
Not 100% certain alas. At least on native (MacOS) I made a small test that starts 2 threads, 1st starts a transaction, 2nd one also, I could see that the 2nd one was blocked until the 1st one finished. I am not sure if this is due to a lock somewhere in the driver, or if SQLite itself handles this. What I think I understand from here is that SQLite handles this itself, but that's not 100% clear to me tbh.
* Add ApolloStore concurrency micro benchmarks * Remove lock from ApolloStore - now individual NormalizedCaches must be thread-safe * Make the call to the next cache be transactional * Add some 'integration test' benchmarks that execute queries
The idea: instead of having a lock in
ApolloStore, let caches handle their own locking, in the hope that this would reduce overall contention (see #4605).Things to note:
nextCacheAdded micro-benchmarks that start a few threads performing read and writes through ApolloStore in parallel (memory / sql / memory then sql).
It looks like the bench performance is improved when comparing the results before/after the change (avg of 6 runs on a Samsung S10+):
A few flame charts
Memory, before:
Memory, after:
Sql, before:
Sql, after:
